fix(storage): surface degraded session database state - #86762

Sound, well-scoped fix. The process-local latch design is the right call — _storage_status_by_path with intentional reset-on-restart so a completed offline repair is adopted without a hidden persistent marker. The StorageDegradedError on write to a latched-store is a clean way to stop a write retry storm. Tests including test_unrepaired_malformed_write_latches_storage_and_pauses_later_writes are good. A couple of notes.

1. The "process-wide" guarantee is per-process, not cross-process

mark_storage_degraded latches into a module-global dict in the current process. Long-running backends may run as more than one process (a gateway writing state.db plus a second gateway, or a desktop poller). If corruption is first observed in process A, only A latches; process B, on its next write, redisovers via its own is_malformed_db_error path and latches independently. The docstring's framing — "Long-running backends create short-lived SessionDBs ... while the gateway keeps another instance" — is all one process, so within-process it's exactly right. Just be aware the guarantee doesn't extend across processes; if two writers exist, the write-retry-storm pause only kicks in after each process individually observes structural failure. Probably acceptable, but worth stating explicitly given "process-wide" reads as stronger than it is.

2. Latch only triggers on write/heal, not on a read-only surface

The latch fires through the write and schema-heal paths (mark_storage_degraded on is_malformed_db_error). A purely read-only SELECT against a malformed DB — the sidebar poll, say — may not hit that path, so the first sighting of corruption by a read-only process won't latch. The desktop banner relies on get_storage_status, which only reports "degraded" if some process latched it. If the first (and only) observer is read-only, the banner stays "ok" until a write occurs. Worth confirming the malformed-repair path also marks on read-side detection.

3. Minor

• get_storage_status returns "ok" for an absent store too (key missing → "ok"). An uninitialized DB is not corrupted, so that's fine, but a caller can't distinguish "genuinely ok" from "never seen." Acceptable given the desktop only needs the degraded red flag.

Tests

Good coverage of the core: latch-on-malformed-write, pause-later-writes, plus the web-server/health-probe wiring in test_web_server.py and the TS hook/store tests including the caller-side storage: 'degraded' → $storageStatus propagation. The gap is cross-process (finding #1) — nothing exercises two independent SessionDB instances/processes seeing corruption.

Blocking finding from re-reviewing the current head (f364df22): the PR's own FTS-rebuild test suite is red, and CI slice 4/12 fails because of it. This is a genuine internal regression, not a pre-existing flake.

What fails: tests/state/test_fts_runtime_rebuild.py — 6 of 11 tests fail (reproduced locally on this head):

• test_second_corruption_fails_open_and_rebuilds_on_reopen
• test_failed_in_place_rebuild_fails_open
• test_stale_search_preserves_not_semantics
• test_existing_peer_observes_fail_open_marker
• test_failed_startup_rebuild_keeps_fts_detached
• test_legacy_inline_fts_fails_open_and_recovers

Root cause: hermes_state.py:4069-4079 now raises StorageDegradedError as soon as a write hits an FTS-corruption error class after an in-place rebuild fails:

if is_malformed_db_error(exc):
    mark_storage_degraded(self.db_path, exc)
    raise StorageDegradedError(...)  # hard-fails persistence

But the whole point of the existing FTS fail-open path (. _try_runtime_fts_rebuild, then . _enter_fts_fail_open) is that when the one-shot rebuild fails, the DB should detach the derived FTS indexes and continue in a degraded-but-working mode, preserving canonical message rows (the warning log at 4218/4226 documents exactly this intent). The new degrade-latch now turns that recovery into a hard StorageDegradedError that blocks all persistence — which is precisely what the fail-open tests assert must not happen.

Notably, the author's own summary says "test_fts_runtime_rebuild.py ... pass (28 passed)" — that's not the case here; 6/11 fail, and CI slice 4/12 is red on this exact file.

Suggested direction: the degraded-surface feature and the FTS fail-open recovery need to coexist. The latch should mark the storage degraded (so the banner/API surface it) without raising past the fail-open recovery — i.e. let _enter_fts_fail_open / the no-FTS degraded mode proceed, and have mark_storage_degraded reflect the partially-degraded state rather than hard-failing the write. That also lines up with my earlier finding #2 (the banner should report degraded on read-side observation, not only when a write hard-fails).

Happy to dig into the exact call-flow if useful.

Re-reviewed the current head (c909b410). The blocking finding from my last pass is resolved, and the two advisory ones are addressed as well.

1. FTS fail-open regression (was blocking) — fixed. The write path at hermes_state.py:5348-5365 now runs the recovery in the right order: _try_runtime_fts_rebuild first, then _enter_fts_fail_open with mark_storage_degraded(..., pause_writes=False) and a continue so the canonical write is retried. StorageDegradedError is only raised when fail-open could not detach the derived indexes — which is the correct semantics: degraded-but-persisting beats blocking persistence on a derived-index failure. The comment at 5351-5353 documents the intent, good.

2. Read-side observation — fixed. Both read paths (pooled connection at ~4955 and the single-conn fallback at ~4985) now mark_storage_degraded(..., pause_writes=False) on is_malformed_db_error, so a read-only observer (sidebar poll, MATCH read) surfaces the degraded state instead of leaving the banner "ok" until some write happens. That closes the gap from my original finding #2.

3. Latch monotonicity. mark_storage_degraded keeps writes_were_paused or pause_writes, so a later fail-open observation can't un-pause a write-paused latch. Correct direction.

Verification. Local run of tests/state/test_fts_runtime_rebuild.py + tests/test_state_db_malformed_repair.py on this head is not meaningful on my machine: SQLite 3.51.0 refuses the shadow-table and writable_schema writes the corruption helpers rely on (table messages_fts_data may not be modified), and the same suites fail identically on current main (36/43) — pre-existing environment limitation, not a PR regression. The authoritative signal is CI on this head: all required checks pass, including the Python tests slice that was red on the Aug 15 head and the macOS-only suite. The repair-path change (hermes sessions repair calling repair_state_db_schema directly, bypassing the write latch) is the right call — the repair tool must not be blocked by the latch it exists to clear.

Blocking finding withdrawn. From my side this is merge-ready.

Reviewed exact head 97b04ab9f4d2ae9f8ea013f031141c13e339cff7 against exact base 63279301bcbdc185c1b07b98a9312eb0c862f26d, including the full 11-file diff, the fresh rebase conflict resolution, existing review history, #72046, the merged prevention/repair work in #71724 and #84882, current-main structural quarantine from #101224 (salvage of #101095, with leomcamilo's authorship preserved), the gateway/agent durability fallbacks that quarantine owns, and the adjacent open quarantine hardening in #102092 / #101303 / #101276. This head is a single commit, so the every-commit gate is the head gate; CI, Docker, and Nix all complete successfully on this exact object.

The earlier FTS regression is genuinely resolved on this rebase: _enter_fts_fail_open() still gets first refusal on derived-index corruption, that path marks the profile visible-degraded with pause_writes=False, and canonical persistence remains available. Bare structural corruption then composes with main's stronger per-handle _halt_db_corrupt() quarantine. The read-side observation path likewise surfaces malformed reads without automatically stopping canonical writes. Those are good separations.

I still see two merge blockers at the current head, both created by the new composition with main rather than by the older review findings.

1. The process-wide pause creates a second permanent-refusal error class, but main's transcript-durability owners do not recognize it.

tests/test_state_db_malformed_repair.py::test_unrepaired_malformed_write_latches_storage_and_pauses_later_writes now proves the important cross-handle case explicitly: handle A observes structural corruption and raises StateDbCorruptError; a fresh handle B for the same path reaches _storage_writes_paused() and raises StorageDegradedError before touching SQLite.

That is exactly where this PR currently falls out of main's quarantine contract:

• gateway/session.py::_append_to_transcript_serialized() diverts to JSONL + pending_messages/ and suppresses the FTS retry only for StateDbReplacedError / StateDbCorruptError. Other failures fall through to the bounded dirty-transcript path and log will retry.
• run_agent.py::_flush_messages_to_session_db() likewise sends the batch to divert_session_transcript_jsonl() only for StateDbReplacedError / StateDbCorruptError.
• classify_persistence_error() recognizes StateDbCorruptError by type and raw malformed/NOTADB text as corrupt, but StorageDegradedError("session database is degraded after a corruption error; persistence is paused...") matches neither and currently classifies as unknown.

So the first handle gets #101224's fail-closed and durable-fallback behavior, while every peer handle stopped by this PR's process-wide latch gets fail-closed SQLite writes but loses the matching fallback/classification semantics. That can put gateway messages back into the in-memory retry queue instead of immediately preserving them on disk, which is precisely the retry/durability half #101224 closed.

Please make the global paused-write result participate in the same terminal persistence contract. The implementation can be inheritance or a shared semantic/base class rather than special-casing one more name, but all three owners above need to agree that this result is corrupt/non-retryable and must divert. I would add two acceptance tests around the fresh peer after latch shape: gateway append => zero FTS rebuild/retry + JSONL/spool diversion; agent flush => JSONL diversion; and a classifier assertion that the peer refusal is corrupt. That tests the actual other side of the new latch rather than only proving that SQLite was not touched.

2. The Desktop recovery instruction now conflicts with the structural-corruption recovery authority in this exact base.

StorageDegradedBanner gives one unconditional instruction: stop profile writers, then run hermes sessions repair, and says historical chats are preserved. But current main deliberately distinguishes two different degraded shapes that this PR collapses into the same storage: "degraded" value:

• derived FTS failure can degrade while canonical writes remain available;
• bare structural SQLITE_CORRUPT / SQLITE_NOTADB can mean canonical B-tree/schema/freelist damage, pauses writes, and main's StateDbCorruptError / docs/state-db-recovery.md lead with hermes sessions recover --source ... --inspect-only (or restore a snapshot), preserving the damaged source and WAL bundle for recovery.

hermes sessions repair is still a valid guarded repair facility, and #84882 correctly serialized that surgery across processes, but it is not the same first-line contract main now emits for a quarantined structural image. Because the banner is the new durable user-facing safety surface, it should not send both states through one stronger claim/action than the backend can justify. Either expose enough mode/reason to render the correct guidance, or make the common copy conservative (no unconditional preservation guarantee; point structural quarantine to the non-destructive inspect/recover path). I would cover the rendered structural case in the Desktop test, not just atom publication.

Interlock/credit check: this is complementary to #71724's corruption-prevention work and #84882's repair serialization, not a duplicate. It also needs to preserve #101224/#101095's structural-quarantine ownership rather than becoming a parallel error protocol. #102092, #101303, and #101276 are adjacent hardening of that same quarantine across maintenance/read-open/housekeeping paths; they are complementary and should compose without changing this PR's status/UI ownership, but whichever lands first should be rechecked for the shared no-touch/no-retry invariant.

The rebase did a substantial amount of hard work correctly: the old FTS fail-open blocker is gone, the new latch is monotonic, the status plumbing is narrow, and the exact head is fully green. The remaining problem is the last ownership hop from “peer write is refused” to “the transcript is durably diverted and the user is told the recovery action main actually owns.” Close those two seams and this becomes a coherent extension of the current corruption architecture.